BREAKING CHANGE: SqlServerDsc: Fix inconsistent parameter names in all resources #917

johlju · 2017-11-28T20:10:51Z

Pull Request (PR) description
This pull request will fix all inconsistent parameter names so that InstanceName is the parameter used to set the name of the instance to configure, and ServerName is used to specify the NetBios or FQDN name where the instance can be found.

This Pull Request (PR) fixes the following issues:
Fixes #308

Task list:

Change details added to Unreleased section of CHANGELOG.md?
Added/updated documentation, comment-based help and descriptions in .schema.mof files where appropriate?
Examples appropriately updated?
New/changed code adheres to Style Guidelines?
Unit and (optional) Integration tests created/updated where possible?

This change is

johlju · 2017-11-28T20:16:32Z

@randomnote1 @nabrond If you have time, then please help me review this one. I really need a second pair of eyes on this one. It's hard to be objective of ones own code. 😄

@PlagueHO If you get bored on your mini-holiday, then you can help out reviewing this one. Haha 😉

codecov-io · 2017-11-28T21:00:43Z

Codecov Report

Merging #917 into dev will not change coverage.
The diff coverage is 99%.

@@         Coverage Diff         @@
##            dev   #917   +/-   ##
===================================
  Coverage    96%    96%           
===================================
  Files        32     32           
  Lines      3515   3515           
===================================
  Hits       3395   3395           
  Misses      120    120

randomnote1 · 2017-11-28T21:59:33Z

Reviewed 15 of 129 files at r1.
Review status: 15 of 129 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

nabrond · 2017-11-29T02:58:05Z

@johlju, I noticed that no helper functions were included in this PR. Functions like Connect-SQLServer are still utilizing the "old" parameters. Do you think it is too ambitious to bring them into scope of this pull?

Comments from Reviewable

nabrond · 2017-11-29T04:49:49Z

Reviewed 7 of 129 files at r1.
Review status: 22 of 129 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

nabrond · 2017-11-29T12:11:16Z

Reviewed 6 of 129 files at r1.
Review status: 28 of 129 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

nabrond · 2017-11-29T12:11:44Z

Reviewed 1 of 129 files at r1.
Review status: 29 of 129 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

johlju · 2017-11-29T19:13:23Z

@nabrond I thought about it, but I didn't want to do that in this PR since it can be done later without a breaking change (was a lot of work getting all the tests working any way 😄 ). But I will make sure to add an issue for that so it doesn't get forgotten.

randomnote1 · 2017-11-30T12:25:26Z

Reviewed 2 of 129 files at r1.
Review status: 31 of 129 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

randomnote1 · 2017-11-30T15:09:06Z

Reviewed 15 of 129 files at r1.
Review status: 44 of 129 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 129 at r1 (raw file):

        Name            = $Name
        ServerName      = $ServerName
        InstanceName = $InstanceName

Fix formatting

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 342 at r1 (raw file):

    $getTargetResourceParameters = @{
        InstanceName = $PSBoundParameters.InstanceName

Fix formatting

Examples/Resources/SqlDatabasePermission/1-GrantDatabasePermissions.ps1, line 23 at r1 (raw file):

            Name                 = 'CONTOSO\SQLAdmin'
            LoginType            = 'WindowsUser'
            ServerName           = 'sqltest.company.local'

Just curious...Why did you update this to an FQDN?

Comments from Reviewable

randomnote1 · 2017-11-30T15:28:21Z

Reviewed 11 of 129 files at r1.
Review status: 55 of 129 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

johlju · 2017-11-30T18:11:39Z

Review status: 55 of 129 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Examples/Resources/SqlDatabasePermission/1-GrantDatabasePermissions.ps1, line 23 at r1 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

Just curious...Why did you update this to an FQDN?

It said 'SQLServer' before so it was messing with my search and replace, and when changing the name I just wrote FQDN by habit. 😄 I can change it back to NetBIOS if you like.

Comments from Reviewable

randomnote1 · 2017-11-30T19:26:52Z

Reviewed 16 of 129 files at r1.
Review status: 71 of 129 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

Examples/Resources/SqlDatabasePermission/1-GrantDatabasePermissions.ps1, line 23 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

It said 'SQLServer' before so it was messing with my search and replace, and when changing the name I just wrote FQDN by habit. 😄 I can change it back to NetBIOS if you like.

Ahh, that makes sense. No big deal. Like I said, I was just curious!

Tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 58 at r1 (raw file):

        $instanceParameters = @{
            InstanceName = 'MSSQLSERVER'
            ServerName = 'Server1'

Hash table should be formatted with the appropriate spacing.

Comments from Reviewable

randomnote1 · 2017-12-01T13:08:55Z

Reviewed 26 of 129 files at r1.
Review status: 97 of 129 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

Comments from Reviewable

randomnote1 · 2017-12-01T13:40:52Z

@johlju, There are only a few formatting issues. Other than that, I think this looks good.

Reviewed 30 of 129 files at r1.
Review status: 127 of 129 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

Comments from Reviewable

randomnote1 · 2017-12-01T13:41:37Z

Review status: 127 of 129 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

Examples/Resources/SqlDatabasePermission/1-GrantDatabasePermissions.ps1, line 23 at r1 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

Ahh, that makes sense. No big deal. Like I said, I was just curious!

Comments from Reviewable

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

- BREAKING CHANGE: Parameter NodeName has been renamed to ServerName (issue dsccommunity#308).

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

- BREAKING CHANGE: Parameter NodeName has been renamed to ServerName (issue dsccommunity#308).

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

- BREAKING CHANGE: Parameter NodeName has been renamed to ServerName (issue dsccommunity#308).

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

- BREAKING CHANGE: Parameters SQLServer has been renamed to ServerName (issue dsccommunity#308).

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

johlju · 2017-12-01T16:12:28Z

Many thanks for reviewing! This was a big one! 😄 All comments should be resolved. Please sign off on them.
Also opened more issues regarding rename, please comment on those to see if we should do them (or not) before release.

Review status: 124 of 129 files reviewed at latest revision, 4 unresolved discussions.

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 129 at r1 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

Fix formatting

Done.

DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 342 at r1 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

Fix formatting

Done.

Examples/Resources/SqlDatabasePermission/1-GrantDatabasePermissions.ps1, line 23 at r1 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

:lgtm:

Done.

Tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 58 at r1 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

Hash table should be formatted with the appropriate spacing.

Done. For some reason auto-formatting did not touch this one. I moved it manually.

Comments from Reviewable

randomnote1 · 2017-12-01T17:57:42Z

Glad to help! Just one more thing I missed earlier and it should be good to go.

Reviewed 2 of 3 files at r2, 2 of 2 files at r3.
Review status: 128 of 129 files reviewed at latest revision, 1 unresolved discussion.

CHANGELOG.md, line 8 at r3 (raw file):

  ([issue #916](https://github.com/PowerShell/SqlServerDsc/issues/916)).
- BREAKING CHANGE: Significant rename to reduce length of resource names
  - See [issue #851](https://github.com/PowerShell/xSQLServer/issues/851) for a

Should be https://github.com/PowerShell/SqlServerDsc/issues here and throughout the rest of the Unreleased section.

Or should it be throughout the whole file?

Comments from Reviewable

johlju · 2017-12-01T19:16:16Z

Good catch - totally missed that! Can you please sign off on the last changes. 😄

Review status: 128 of 129 files reviewed at latest revision, 1 unresolved discussion.

CHANGELOG.md, line 8 at r3 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

Should be https://github.com/PowerShell/SqlServerDsc/issues here and throughout the rest of the Unreleased section.

Or should it be throughout the whole file?

Done.

Comments from Reviewable

randomnote1 · 2017-12-01T19:40:30Z

Review status: 128 of 129 files reviewed at latest revision, 1 unresolved discussion.

CHANGELOG.md, line 8 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Done.

Still says xSQLServer on this line

Comments from Reviewable

johlju · 2017-12-01T19:43:07Z

Review status: 128 of 129 files reviewed at latest revision, 1 unresolved discussion.

CHANGELOG.md, line 8 at r3 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

Still says xSQLServer on this line

Done. Really this time. 😄 I looked over this twice to make sure, and still managed to miss one. :s

Comments from Reviewable

randomnote1 · 2017-12-01T19:43:25Z

Looks good!

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

johlju · 2017-12-01T19:44:36Z

@randomnote1 Awesome thank you! Waiting for the tests to pass and I will merge this one.

johlju added the needs review The pull request needs a code review. label Nov 28, 2017

johlju added 13 commits December 1, 2017 17:04

Changes to SqlAG

4f3b010

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

Changes to SqlServerLogin

5efdcc3

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

Changes to SqlServerPermission

91ea216

- BREAKING CHANGE: Parameter NodeName has been renamed to ServerName (issue dsccommunity#308).

Changes to SqlServerEndpoint

d6513cc

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

Updated examples

13b91f9

Fix bugs

6268d3d

Fix bugs in SqlAG tests

e283953

Changes to SqlAGDatabases

1baf0cd

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

Changes to SqlAGReplica

c7a8630

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

Fix bugs

db79290

Changes to SqlServerOwner

7489e85

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

Changes to SqlAGListener

87fbb76

- BREAKING CHANGE: Parameter NodeName has been renamed to ServerName (issue dsccommunity#308).

Changes to SqlAlwaysOnService

2f2f327

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

johlju added 15 commits December 1, 2017 17:04

Changes to SqlDatabaseRecoveryModel

c65a7b8

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

Changes to SqlDatabaseRole

468b82a

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

Fix integration tests

585ac40

Updated CHANGELOG.md

cf8165f

Changes to SqlServerConfiguration

35a8214

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

Changes to SqlServerEndpointPermission

30ae6fa

- BREAKING CHANGE: Parameter NodeName has been renamed to ServerName (issue dsccommunity#308).

Changes to SqlServerEndpointState

85945a2

- BREAKING CHANGE: Parameter NodeName has been renamed to ServerName (issue dsccommunity#308).

Changes to SqlServerMaxDop

790032d

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

Changes to SqlServerMemory

69fc0bf

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

Changes to SqlServerNetwork

db58835

- BREAKING CHANGE: Parameters SQLServer has been renamed to ServerName (issue dsccommunity#308).

Changes to SqlServerRole

31103de

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

Changes to SqlServerServiceAccount

517981d

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed to ServerName and InstanceName respectivly (issue dsccommunity#308).

Replaced 'SQLServer' with better option in examples and help texts

f100ba7

Fix CHANGELOG.md

b2e93fb

Fix review comments at r1

6825603

johlju force-pushed the fix-issue-308 branch from be8b332 to 6825603 Compare December 1, 2017 16:07

Fix review comments at r3

e3e354f

Fix review comments at r4

107cbf9

johlju merged commit af10f71 into dsccommunity:dev Dec 1, 2017

vors removed the needs review The pull request needs a code review. label Dec 1, 2017

johlju deleted the fix-issue-308 branch December 5, 2017 15:19

johlju added the breaking change When used on an issue, the issue has been determined to be a breaking change. label Dec 7, 2017

johlju changed the title ~~SqlServerDsc: Fix inconsistent parameter names in all resources~~ BREAKING CHANGE: SqlServerDsc: Fix inconsistent parameter names in all resources Dec 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING CHANGE: SqlServerDsc: Fix inconsistent parameter names in all resources #917

BREAKING CHANGE: SqlServerDsc: Fix inconsistent parameter names in all resources #917

johlju commented Nov 28, 2017 •

edited by joeyaiello

Loading

johlju commented Nov 28, 2017 •

edited

Loading

codecov-io commented Nov 28, 2017 •

edited

Loading

randomnote1 commented Nov 28, 2017

nabrond commented Nov 29, 2017

nabrond commented Nov 29, 2017

nabrond commented Nov 29, 2017

nabrond commented Nov 29, 2017

johlju commented Nov 29, 2017

randomnote1 commented Nov 30, 2017

randomnote1 commented Nov 30, 2017

randomnote1 commented Nov 30, 2017

johlju commented Nov 30, 2017

randomnote1 commented Nov 30, 2017

randomnote1 commented Dec 1, 2017

randomnote1 commented Dec 1, 2017

randomnote1 commented Dec 1, 2017

johlju commented Dec 1, 2017

randomnote1 commented Dec 1, 2017

johlju commented Dec 1, 2017

randomnote1 commented Dec 1, 2017

johlju commented Dec 1, 2017

randomnote1 commented Dec 1, 2017

johlju commented Dec 1, 2017

BREAKING CHANGE: SqlServerDsc: Fix inconsistent parameter names in all resources #917

BREAKING CHANGE: SqlServerDsc: Fix inconsistent parameter names in all resources #917

Conversation

johlju commented Nov 28, 2017 • edited by joeyaiello Loading

johlju commented Nov 28, 2017 • edited Loading

codecov-io commented Nov 28, 2017 • edited Loading

Codecov Report

randomnote1 commented Nov 28, 2017

nabrond commented Nov 29, 2017

nabrond commented Nov 29, 2017

nabrond commented Nov 29, 2017

nabrond commented Nov 29, 2017

johlju commented Nov 29, 2017

randomnote1 commented Nov 30, 2017

randomnote1 commented Nov 30, 2017

randomnote1 commented Nov 30, 2017

johlju commented Nov 30, 2017

randomnote1 commented Nov 30, 2017

randomnote1 commented Dec 1, 2017

randomnote1 commented Dec 1, 2017

randomnote1 commented Dec 1, 2017

johlju commented Dec 1, 2017

randomnote1 commented Dec 1, 2017

johlju commented Dec 1, 2017

randomnote1 commented Dec 1, 2017

johlju commented Dec 1, 2017

randomnote1 commented Dec 1, 2017

johlju commented Dec 1, 2017

johlju commented Nov 28, 2017 •

edited by joeyaiello

Loading

johlju commented Nov 28, 2017 •

edited

Loading

codecov-io commented Nov 28, 2017 •

edited

Loading